-
Notifications
You must be signed in to change notification settings - Fork 351
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Revert PR #1654 #1663
Revert PR #1654 #1663
Conversation
…)" This reverts commit c04d275.
GeraldRequired Reviewers
Don't want to be involved in this pull request? Comment |
npm Snapshot: PublishedGood news!! We've packaged up the latest commit from this PR (99a52a9) and published it to npm. You Example: yarn add @khanacademy/perseus@PR1663 If you are working in Khan Academy's webapp, you can run: ./dev/tools/bump_perseus_version.sh -t PR1663 |
Size Change: -156 B (-0.02%) Total Size: 863 kB
ℹ️ View Unchanged
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1663 +/- ##
==========================================
- Coverage 70.89% 70.68% -0.22%
==========================================
Files 562 587 +25
Lines 108291 112784 +4493
Branches 7981 12007 +4026
==========================================
+ Hits 76774 79717 +2943
- Misses 31330 33067 +1737
+ Partials 187 0 -187 see 1148 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
This change was previously implemented in PR #1654, then reverted in #1663 due to a bug. This PR reimplements the change without the bug. The original PR summary follows. --- Previously, we were passing the entire Mafs graph state to onChange. Since the onChange callback data is used by the Interactive Graph widget editor to set the correct answer for the graph, a bunch of extra data like "hasBeenInteractedWith": true was getting saved in the correct field of the Widget data. This extra data is not used, so its presence in datastore is potentially confusing to future maintainers trying to understand the data. It might also cause bugs in the future - e.g. if some code merges it into another object in which those properties are meaningful. This PR ensures that we only save relevant data in the correct field of interactive graph widgets. Issue: none ## Test plan: `yarn dev; open http://localhost:5173` Play around with the graphs on the dev UI gallery page. In particular: - Unlimited point - Polygons with angle and side labels - Polygons with side snapping You should not observe any issues. Edit, save and publish an interactive graph widget in an exercise. As a learner, you should be able to complete the exercise successfully. Author: benchristel Reviewers: jeremywiebe, nicolecomputer Required Reviewers: Approved By: jeremywiebe Checks: ✅ Publish npm snapshot (ubuntu-latest, 20.x), ✅ Lint, Typecheck, Format, and Test (ubuntu-latest, 20.x), ✅ Cypress (ubuntu-latest, 20.x), ✅ Check for .changeset entries for all changed files (ubuntu-latest, 20.x), ✅ Publish Storybook to Chromatic (ubuntu-latest, 20.x), ✅ Check builds for changes in size (ubuntu-latest, 20.x), ✅ gerald Pull Request URL: #1664
The linked PR caused a bug where certain interactive graph types would lose key features
after you interacted with them:
The cause was that the interactive graph widget was passing incomplete data to
its onChange callback. The Renderer stored this data in its state, overwriting the
original widget config. The borked data then got passed back down to the widget on the
next render, which caused the features to disappear.
Issue: none
Test plan:
yarn dev; open http://localhost:5173
Turn on Mafs for the unlimited point and polygon graphs. Play with the graphs and
confirm that everything looks OK.